Skip to content

Solution#501

Open
PolianskyiDmytro wants to merge 3 commits into
mate-academy:masterfrom
PolianskyiDmytro:develop
Open

Solution#501
PolianskyiDmytro wants to merge 3 commits into
mate-academy:masterfrom
PolianskyiDmytro:develop

Conversation

@PolianskyiDmytro

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core requirement is not met: when the source and destination refer to the same location, the app must do nothing, but your current implementation prints an error instead. In src/app.js, around the copy function (approximately lines 20–30, where you compare the source and destination paths), you currently check equality and log an error; this should instead be an early return with no console output when the paths resolve to the same file. Additionally, that check appears to compare the raw argument strings (e.g. src === dest), so cases like ./file.txt vs file.txt or symlinks pointing to the same file are not handled correctly; you should first normalize/resolve both paths (for example with fs.realpathSync or path.resolve + fs.realpathSync) and then compare the resolved values. This behavior is critical because the specification explicitly states that the app "must do nothing" for same-location copies, and tests are likely to assert both the lack of output and the absence of a copy attempt.

Apart from this, your handling of argument counts, file existence checks, and distinguishing files from directories looks solid and uses console.error appropriately for other error cases. Once you adjust the same-location check to silently no-op using resolved paths, the implementation should align well with the stated requirements. Keep those changes focused and minimal, and your solution will be in good shape.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/app.js Outdated
Comment on lines +15 to +18
if (src === dest) {
console.error('Source and destination paths cannot be the same.');

return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check prints an error but the requirement says the app "must do nothing" when source and destination are the same. Instead of logging an error, simply return with no output. Also consider resolving paths (fs.realpathSync or path.resolve) before comparing so different path forms that point to the same file are detected.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because the handling of the “same source and destination” case does not meet the task requirements and will cause tests to fail. In src/app.js, in the block where you compare the source and destination arguments directly (e.g., if (src === dest)), you currently log an error and return, but the specification requires that when source and destination refer to the same file, the program must silently do nothing with no console output [CHECKLIST ITEM #2, #7]. To satisfy this, you should resolve/normalize both paths: require path (const path = require('path')), then use fs.realpathSync on the source and, for the destination, use fs.realpathSync if it exists, otherwise path.resolve(dest) (since realpathSync throws for non-existent paths) before comparing them. If the resolved paths match, perform an early return without any console.error or console.log, ensuring absolutely no output for that case [CHECKLIST ITEM #8]. This is a critical behavioral requirement rather than a style issue, because the tests will explicitly check for a silent no-op when copying a file onto itself via the same or equivalent path. Aside from this, your argument validation, file existence checks, and error reporting behavior look solid, so once you fix the same-location logic the implementation should align well with the task. Keep going—this is a focused change and a good opportunity to practice careful path handling with realpathSync and path.resolve.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/app.js
Comment on lines +39 to +42
if (src === dest) {
console.error('Source and destination paths cannot be the same.');

return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checklist items #2, #7 and #8 require that when source and destination refer to the same file the app must do nothing (no console output). Here you log an error and return. Instead, resolve/normalize both paths (for example with fs.realpathSync or path.resolve + fs.realpathSync) and perform a silent early return when they match.

Comment thread src/app.js
/* eslint-disable no-console */
'use strict';

const fs = require('fs');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To compare resolved paths correctly and handle symlinks, also require path (e.g. const path = require('path')) and use fs.realpathSync on the source and, if the destination exists, on the destination too. If the destination does not exist use path.resolve(dest) rather than realpath (realpathSync throws for non-existent paths).

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved as an educational override so you can keep progressing while you refine a few remaining details. Overall, you did well handling argument validation, checking that the source exists and is a file, and performing the actual copy with appropriate console.error calls for error reporting.

The main critical issue is the same-file handling in src/app.js where you currently compare paths with if (src === dest) (on the line with that condition): the requirement is to resolve/normalize both paths using fs.realpathSync(src) for the source and fs.realpathSync(dest) if the destination exists, otherwise path.resolve(dest), then return early with absolutely no console output when those resolved paths are equal [CHECKLIST ITEM #2, #8]. To support this, you also need to require('path') at the top of src/app.js (for example, const path = require('path')) and use it specifically in the fallback case when the destination does not yet exist. As a minor improvement, consider wrapping fs.readFileSync(src) in a try/catch to handle read errors more gracefully and think about removing or avoiding extra console.log output on success so tests expecting silence aren’t affected.

These adjustments will make your same-file detection fully robust and aligned with the specification, and the rest of your implementation shows solid progress—keep going, you’re very close to a complete, production-quality solution.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/app.js
/* eslint-disable no-console */
'use strict';

const fs = require('fs');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to require the path module (e.g. const path = require('path')) because the specification requires using path.resolve(dest) when the destination does not exist before comparing paths.

Comment thread src/app.js
Comment on lines +39 to +40
if (src === dest) {
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The raw comparison if (src === dest) is insufficient. Per the guideline, resolve/normalize both paths: use fs.realpathSync(src) for the source (after verifying it exists) and for the destination use fs.realpathSync(dest) if it exists, otherwise path.resolve(dest). If the resolved paths are equal, perform an early return with NO console output. This implements the required silent no-op for same-file cases (checklist items #2 and #8).

@brespect brespect left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants